fix: bell mode regression, plus adversarial review and dark-mode follow-ups - #138
Merged
Conversation
`customPatternDurationLimits` is now a four-tuple. `normalizePersistedSettingsState` maps over it to build the steps, so its length is what makes the `as CustomPatternSteps` cast true. As a plain array it did not: adding a fifth entry produced a five-element result still typed as a four-tuple, and `buildStepsMetadata` would then read `undefined` for a duration and let `Animated.timing` fall back to its own default. Type-level only, no runtime change. The storage read retried in the same tick, so it could only survive a bridge hiccup — not the locked database its own comment claimed. It now pauses first. `animate()` coalesces the easing instead of assigning it above the spread. A caller passing an explicit `undefined` used to fall through to React Native's `Easing.inOut(Easing.ease)` rather than the app's quad curve. No caller does that today; this keeps the default true regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ettings text Two findings from an adversarial review. The bell remap was a net regression. `buildStepsMetadata` gives the audio id `hold` to both the step after the inhale and the step after the exhale, so the bell assigned to it sounded twice per cycle. On Square — the default pattern — the sequence became bell1, bell1, bell2, bell1, which puts three identical bells in a row across the loop boundary. It fixed the patterns with no holds and broke the ones with holds, including the default, and the test asserted the broken behaviour. Bell mode now cues only the two direction changes and leaves the holds silent, so every pattern alternates. Each cue player is built independently instead of all-or-nothing, which is what lets a mode cue some steps and not others. Tests cover a full Square cycle rather than only the asset mapping. The iOS settings screen still read `slate-500` directly, so its secondary text measured 3.07:1 on the dark card — worse than anything the earlier contrast commit fixed, and it is the longest body text in the app. It now reads `useThemeColors()` like everything else. `hideBottomBorder` becomes `hideBottomBorderWeb`: only the web implementation honours it, and the old platform-neutral name promised parity that a TypeScript contract cannot enforce. The parity test comment claimed the contract could require an accessibility label; it cannot, and now says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`{ stopTogether: false }` was doing no work. The 0.35 ratio already keeps both
label fades inside the step, so nothing interrupts the circle — verified across
every reachable step length. What the flag did add was a silent failure: an
interrupted circle reported `finished: true`, so `loopAnimations` would step on
forever with a frozen circle instead of stopping. A loud failure is the right one
in the path that drives the whole exercise.
Audio now also stops when the app returns to `active`. `expo-audio` pauses the
players it interrupts and replays them afterwards, and the JS `AppState` event
arrives after its native observers have run — so a cue caught mid-word finished
in the middle of the wrong step, seconds or minutes later. This is the cost of
pausing only on a real background, and it is now paid deliberately. An iOS
interruption that begins and ends without an `AppState` transition is still
uncovered; expo-audio exposes no JS event for it.
The accessibility hooks ask their question at module load instead of on first
mount, so the answer is in flight before anything renders, and a change event
that lands while the query is outstanding is no longer overwritten by the stale
answer — which, because the value is cached at module scope, used to poison every
later mount in the process.
The paused screen announces itself and carries a header role. The step
announcements just stopped, so a screen-reader user was told nothing, while the
completion screen already announced.
`useKeepAwake` no longer covers the completion screen, which never dismisses
itself and would hold the display on until the user came back to the phone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ance The iOS large title and the picker wheel are UIKit, and UIKit takes its colours from the system appearance. A user who turned "Use system theme" off and chose Dark on a light phone got a near-black title on the app's dark background, and a picker drawn in light-mode chrome — both barely legible. Override the app's own appearance instead of colouring each native view by hand: every native view then follows the chosen theme, including any added later. "unspecified" hands control back when the user is following the system, so that path is unchanged. React Native's own documentation confirms this affects the app only and never the system UI. Worth noting for anyone reading `colors.test.ts`: it asserts the contrast of colours React Native draws, and can say nothing about what UIKit paints. This class of bug is invisible to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #137, which merged while three adversarial reviewers were still running. They found two problems in that merge, so master currently carries a user-facing regression. This branch fixes it.
bun run validate:staticpasses (81 tests). The full Maestro suite passes on both platforms against Release builds — Android with R8, iOS re-run after the startup change. The iOS dark-mode fix was verified on device by reproducing the exact failing configuration.The regression in master
In
origin/mastertoday:buildStepsMetadatagives the audio idholdto both the step after the inhale and the step after the exhale, so any bell assigned to it sounds twice per cycle:#137 fixed the patterns with no holds and broke the ones with holds — including the default — and added a test asserting the broken mapping.
Bell mode now cues only the two direction changes and leaves the holds silent, so every pattern alternates. Cue players are built independently rather than all-or-nothing, which is what lets a mode cue some steps and not others. The test walks a full Square cycle instead of checking the asset map.
Native views ignored the chosen theme
Reported from a screenshot: with "Use system theme" off and Dark chosen on a light phone, the iOS large title rendered near-black on the app's dark background, and the picker wheel was drawn in light-mode chrome.
Both are UIKit, and UIKit follows the system appearance. Rather than colour each native view by hand, the app now overrides its own appearance with
Appearance.setColorScheme, so every native view follows the chosen theme — including any added later."unspecified"hands control back when the user is following the system.Worth flagging for reviewers:
colors.test.tsasserts the contrast of colours React Native draws and is structurally blind to anything UIKit paints. Three reviewers and I all missed this; it took looking at the screen.{ stopTogether: false }was doing no work, and cost the loop its failure signalThe reviewer rebuilt the composition without the flag and ran every reachable step length — all reach their target and report finished. The 0.35 ratio alone fixes the short-step bug.
What the flag added was a silent failure: an interrupted circle reported
finished: true, soloopAnimationswould step on forever with a frozen circle instead of stopping. Removed. The 25 exercise tests still pass, which is the proof it was redundant.iOS audio interruptions
The answer to "does pausing only on
backgroundmean the app talks over a phone call?" is no — expo-audio deactivates the session. But two real consequences fell out:AppStateevent arrives after expo-audio's native observers, making the existingstopExerciseAudio()too lateAudio now also stops on the return to
active, which covers both. Still uncovered: an interruption that begins and ends without anAppStatetransition. expo-audio exposes no JS event for it, so that is a known gap rather than an oversight.Smaller review findings
customPatternDurationLimitsis a four-tuple.normalizePersistedSettingsStatemaps over it, so its length is what makes theas CustomPatternStepscast true — as a plain array it was simply untrue, and a fifth entry would have produced a five-element result still typed as four.animate()coalesces its easing, so an explicitundefinedno longer falls through to React Native's default curve.useKeepAwakeno longer covers the completion screen, which never dismisses itself and held the display on until the user came back.slate-500— 3.07:1 on the dark card, the longest body text in the app.hideBottomBorder→hideBottomBorderWeb: only web honours it, and the platform-neutral name promised parity a TypeScript contract cannot enforce.Known gaps, deliberately not fixed
@expo/uiAPI cannot express it:semanticstakes onlycontentType,FilledTonalButtonexposes no content description, andIconneeds a vector drawable. Documented at the call site.dark.imagedoes nothing on Android — the plugin only writes a dark drawable whendrawable.darkIconis set. No visible fault, because that splash icon is transparent and the dark colour does apply.splash-dark.pngships ~28 KB into the web bundle, whereSplashScreenManagerreturns early and never renders it.